Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use oz-upgrades-unsafe-allow-reachable in ERC1967Upgrade #3971

Merged
merged 8 commits into from
Jan 19, 2023
Merged

Use oz-upgrades-unsafe-allow-reachable in ERC1967Upgrade #3971

merged 8 commits into from
Jan 19, 2023
+14 −2

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 18, 2023

This is needed to work with the latest version of the upgrade plugin.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx requested a review from frangio January 18, 2023 21:10
@frangio
Copy link
Contributor

frangio commented Jan 18, 2023

This is needed to work with the latest version of the upgrade plugin.

A correction, this isn't needed, but it's an improvement to this library because the unsafe-allow is used more granularly.

I think this PR is doing the opposite of what we want. These functions by themselves are not safe. I was thinking it should be _upgradeToAndCallUUPS that should have unsafe-allow-reachable delegatecall, but the proxiableUUID check is not what makes it safe. What makes it safe is the onlyProxy modifier in UUPSUpgradeable, so the annotation should be placed in UUPSUpgradeable.upgradeTo[AndCall].

@frangio
Copy link
Contributor

frangio commented Jan 18, 2023

Looks good! I would actually update the changelog, it's important to mention that this requires a recent version of Upgrades Plugins (@openzeppelin/upgrades-core@1.21.0).

@Amxx Amxx mentioned this pull request Jan 18, 2023
Closed
3 tasks
@Amxx Amxx marked this pull request as ready for review January 19, 2023 13:55
@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 654ef38

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@frangio frangio enabled auto-merge (squash) January 19, 2023 21:53
@frangio frangio merged commit c404862 into master Jan 19, 2023
@frangio frangio deleted the Amxx-patch-1 branch January 19, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frangio frangio frangio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Amxx @frangio